-
Notifications
You must be signed in to change notification settings - Fork 867
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add metrics & micrometer support to spring-boot-autoconfigure #6270
Add metrics & micrometer support to spring-boot-autoconfigure #6270
Conversation
implementation("io.opentelemetry:opentelemetry-micrometer1-shim") { | ||
// just get the instrumentation, without the micrometer itself | ||
exclude("io.micrometer", "micrometer-core") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
instrumentation/micrometer/micrometer-1.5/javaagent/build.gradle.kts
Outdated
Show resolved
Hide resolved
instrumentation/spring/spring-boot-autoconfigure/build.gradle.kts
Outdated
Show resolved
Hide resolved
...emetry/instrumentation/spring/autoconfigure/exporters/logging/LoggingExporterProperties.java
Outdated
Show resolved
Hide resolved
import org.springframework.boot.context.properties.ConfigurationProperties; | ||
|
||
/** Configuration for {@link io.opentelemetry.exporter.logging.LoggingSpanExporter}. */ | ||
@ConfigurationProperties(prefix = "otel.exporter.logging") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we open sdk issue to add these as sdk autoconfigure properties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really think so - the spring starter uses a bit different configuration scheme than the SDK (which I dislike, I think they should use exactly the same property names), and the exporters are enabled/disabled in a bit different way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fully support changing the sprint boot configuration to match sdk configuration if that's possible
void noProperties() { | ||
runner.run( | ||
context -> | ||
assertThat(context.getBean("otelLoggingMetricExporter", LoggingMetricExporter.class)) | ||
.isNotNull()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do u think this behavior is expected, that the logging exporter is enabled by default?
even if it requires adding the logging exporter as a dependency, I could see that being something handy to have on the classpath for debugging but still not enabled by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I think that there's definitely something broken in all exporter configurations, between the "enabled by default" and @ConditionalOnMissingBean
annotations placed on every other bean. I'd like to tackle it across all exporters in another PR though (and maybe do #923 along the way)
Co-authored-by: Trask Stalnaker <[email protected]>
Our Spring Boot starter completely did not support metrics, so I decided to add them.